-
Notifications
You must be signed in to change notification settings - Fork 841
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: added _Nonnull and _Nullable attributes to mujoco.h #1038
base: main
Are you sure you want to change the base?
feat: added _Nonnull and _Nullable attributes to mujoco.h #1038
Conversation
MJAPI void mj_inverseSkip(const mjModel* m, mjData* d, int skipstage, int skipsensor); | ||
|
||
MJAPI void mj_inverseSkip(const mjModel* NONNULL_ARG m, mjData* NONNULL_ARG d, | ||
int skipstage, int skipsensor) NONNULL_FUNC(1, 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after running -
➜ mujoco git:(309-Adding-_Nonull-and-_Nullable-attributes-to-mujoco.h) git clang-format HEAD~1
changed files:
include/mujoco/mjmacro.h
include/mujoco/mujoco.h
➜ mujoco git:(309-Adding-_Nonull-and-_Nullable-attributes-to-mujoco.h) git clang-format HEAD~1
changed files:
include/mujoco/mujoco.h
The asterisk * is becoming insistent with the rest of the code for the pointers in the updated definitions.
Moreover I am looking at the conflict which is going with the mujoco.h file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the conflict between a version of the file that uses the NONNULL_ARG and NONNULL_FUNC macros, and a version that does not use those macros?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will look into the changes with the comments in the later part of the mujoco.h file in the morning.
The changes I made are just before the ray-collisions. =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preliminary review, until @saran-t takes a look 🙂
MJAPI int mj_saveLastXML(const char* filename, const mjModel* m, char* error, int error_sz); | ||
MJAPI int mj_saveLastXML(const char* NONNULL_ARG filename, | ||
const mjModel* NONNULL_ARG m, char* NULLABLE_ARG error, | ||
int error_sz) NONNULL_FUNC(1, 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's always have NONNULL (formerly known as NONNULL_FUNC) on its own line?
//---------------------------------------------------- | ||
|
||
#ifdef __clang__ | ||
#define NONNULL_ARG _Nonnull |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please indent the
#ifdef
bodies. - I don't think we need NONNULL_ARG. Clang supports the GCC syntax.
- Due to 2, let's rename
NONNULL_FUNC
->NONNULL
andNULLABLE_ARG
->NULLABLE
. - I think this whole block belongs in mujoco.h and requires
#undef
s at the end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do it and also I will restore the other unintentional changes that happened.
The basics
git clang-format
The details
Resolves
mujoco.h
APIs #309Proposed Changes
New compilers, such as clang starts to support _Nullable attribute: https://clang.llvm.org/docs/AttributeReference.html#nullable
These can be #define away on unsupported compilers (For example, on GCC, you can define that as attribute((nonnull)): https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
Behavior Before Change
When using MuJoCo API, it is a bit harder to parse either automatically or from documentation when a parameter can be NULL or not. For example, mj_step requires mjData* d to be nonnull, while mj_copyData can take NULL mjData* dest. Similarly, mjv_makeScene can take NULL const mjModel* m. There seems to be no systematic way to understand which parameter can be NULL which cannot.
Behavior After Change
These nullability attributes can be helpful when generating interface glue code to MuJoCo library. For example, swift-mujoco right now assumes all fields to be nonnull and have to manually implement selected APIs that can be NULL. If nullability attribute added, these can be automatically generated like the rest of APIs.
Reason for Changes
When using MuJoCo API, it is a bit harder to parse either automatically or from documentation when a parameter can be NULL or not.
Test Coverage
I haven't tested this, as soon as I got confirmation that these are the required changes that will update the status of the Test Coverage.
Documentation
Additional Information